Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change CI Runners #1630

Merged
merged 12 commits into from
Feb 8, 2023
Merged

Change CI Runners #1630

merged 12 commits into from
Feb 8, 2023

Conversation

alvicsam
Copy link
Contributor

@alvicsam alvicsam commented Jan 30, 2023 •
edited
Loading

PR changes ci runners for dynamically created ones

cc https://github.com/paritytech/ci_cd/issues/730

@alvicsam alvicsam marked this pull request as draft January 30, 2023 15:58
@HCastano HCastano changed the title [ci] Change runners Change CI Runners Jan 30, 2023
@HCastano HCastano added the A-CI Continuous integration work item label Jan 30, 2023
@alvicsam
Copy link
Contributor Author

alvicsam commented Feb 6, 2023

@cmichi @HCastano @ascjones @SkymanOne
It seems that I need some help. The test flipper::e2e_tests::it_works is flaky on the new runners and I don't know how can I debug it: https://gitlab.parity.io/parity/mirrors/ink/-/jobs/2354938
Sometimes it works: https://gitlab.parity.io/parity/mirrors/ink/-/jobs/2349676 but I don't see correlation between fail and success runs.

In theory there should be no difference between new runners and old runners because everything executes in docker and the environment is the same.

@ascjones
Copy link
Collaborator

ascjones commented Feb 6, 2023

I believe #1642 will fix this: hope to get it merged soon.

@ascjones
Copy link
Collaborator

ascjones commented Feb 6, 2023

In theory there should be no difference between new runners and old runners because everything executes in docker and the environment is the same.

The reason it is failing is because some other test/process is running in parallel using the alice account, indicated by the following message:

"The transaction has too low priority to replace another transaction already in the pool."

It means that sometimes another transaction for alice gets there first, which is why it fails intermittently.

Having said that, it doesn't explain why it is happening now and not previously.

However, whatever the cause of the interference with the running substrate-contracts-node, it should be fixed by #1642, because that launches an instance of substrate-contracts-node per-test so they will no longer interfere with each other.

@alvicsam
Copy link
Contributor Author

alvicsam commented Feb 6, 2023

@ascjones okay, thank you! Waiting for #1642 then

@ascjones
Copy link
Collaborator

ascjones commented Feb 7, 2023

@ascjones okay, thank you! Waiting for #1642 then

It's in, so try merging master and see what happens!

@alvicsam
Copy link
Contributor Author

alvicsam commented Feb 7, 2023

bot rebase

@alvicsam alvicsam marked this pull request as ready for review February 7, 2023 14:05
@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2023 •
edited
Loading

Codecov Report

Merging #1630 (5d4e7ee) into master (8bc8593) will increase coverage by 24.74%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1630       +/-   ##
===========================================
+ Coverage   45.92%   70.66%   +24.74%     
===========================================
  Files         206      206               
  Lines        6393     6406       +13     
===========================================
+ Hits         2936     4527     +1591     
+ Misses       3457     1879     -1578
Impacted Files Coverage Δ
crates/allocator/src/bump.rs 85.95% <0.00%> (-2.29%) ⬇️
crates/ink/ir/src/ir/attrs.rs 82.02% <0.00%> (+3.76%) ⬆️
crates/ink/ir/src/ir/trait_def/item/mod.rs 88.61% <0.00%> (+4.87%) ⬆️
crates/metadata/src/layout/mod.rs 80.83% <0.00%> (+5.00%) ⬆️
crates/env/src/engine/off_chain/test_api.rs 64.70% <0.00%> (+5.88%) ⬆️
crates/env/src/api.rs 34.78% <0.00%> (+8.69%) ⬆️
crates/ink/ir/src/ir/idents_lint.rs 71.42% <0.00%> (+9.52%) ⬆️
crates/storage/traits/src/layout/impls.rs 10.00% <0.00%> (+10.00%) ⬆️
crates/env/src/engine/off_chain/impls.rs 46.28% <0.00%> (+10.85%) ⬆️
crates/ink/ir/src/ir/item_impl/constructor.rs 91.93% <0.00%> (+11.29%) ⬆️
... and 50 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@alvicsam
Copy link
Contributor Author

alvicsam commented Feb 7, 2023

@ascjones now it works, thank you

@alvicsam alvicsam merged commit 42734a9 into master Feb 8, 2023
@alvicsam alvicsam deleted the as-ci-runner branch February 8, 2023 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateo-moon mateo-moon mateo-moon approved these changes

@sergejparity sergejparity sergejparity approved these changes

@cmichi cmichi Awaiting requested review from cmichi cmichi is a code owner

@ascjones ascjones Awaiting requested review from ascjones ascjones is a code owner

@HCastano HCastano Awaiting requested review from HCastano

@SkymanOne SkymanOne Awaiting requested review from SkymanOne

Assignees
No one assigned
Labels
A-CI Continuous integration work item
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alvicsam @ascjones @codecov-commenter @sergejparity @mateo-moon @HCastano